cryptonote_core: support RandomX V2 and commitments in v17#10038
cryptonote_core: support RandomX V2 and commitments in v17#10038jeffro256 wants to merge 2 commits intomonero-project:masterfrom
Conversation
See docs/BLOCK_HASHING.md This commit does required consensus changes only.
jeffro256
force-pushed
the
randomx_commitment
branch
from
aa54607 to
51df282
Compare
jeffro256
changed the title
| crypto::hash tree_root_hash = get_tx_tree_hash(b); | ||
| blob.append(reinterpret_cast<const char*>(&tree_root_hash), sizeof(tree_root_hash)); | ||
| blob.append(tools::get_varint_data(b.tx_hashes.size()+1)); | ||
| if (b.major_version < HF_VERSION_POW_COMMITMENT) |
There was a problem hiding this comment.
Why do you stop including the number of transactions into the hashing blob? This field is used by XMRig, for display purposes.
More importantly, it's a piece of data that gets erased by including only the tree root hash.
Less data that goes into hashing is worse than more data.
There was a problem hiding this comment.
I removed it because it isn't necessary for consensus and bloats the hashing blob size. If we chose to implement header-only syncing, the varint per hashing blob would impose a non-trivial cost. It shouldn't affect security AFAIK because Keccak isn't susceptible to length extension attacks, but please correct me if wrong.
There was a problem hiding this comment.
Hashing blob is 75 bytes (76-77 with the tx count). It's not a big difference, and apart for using it to display tx count in XMRig, it's also useful for comparing different hashing blobs between different Monero pools. Things like "which pools use the same Monero node, how many different Monero nodes does a pool use, is it mining empty blocks or not, and so on".
There was a problem hiding this comment.
Oh, and one more important thing - I'm pretty sure the "76 bytes minimum" is hard-coded in countless pool implementations in many different places, so it's better to not touch it.
There was a problem hiding this comment.
... and apart for using it to display tx count in XMRig, it's also useful for comparing different hashing blobs between different Monero pools. Things like "which pools use the same Monero node, how many different Monero nodes does a pool use, is it mining empty blocks or not, and so on".
I don't think that the number of txs is a very good metric because it can be easily faked. The miner can determine that there's 25 txs in the tx hash list, but that doesn't mean that there's 25 real txs in the hash list; they could all be generated by the pool itself, and of course the fee goes back to the miner.
Oh, and one more important thing - I'm pretty sure the "76 bytes minimum" is hard-coded in countless pool implementations in many different places, so it's better to not touch it.
Why can't they change it to a 75 byte minimum?
There was a problem hiding this comment.
https://github.com/SChernykh/p2pool/blob/master/src/block_template.cpp#L1303
https://github.com/SChernykh/p2pool/blob/master/src/stratum_server.cpp#L140
https://github.com/xmrig/xmrig/blob/master/src/base/tools/cryptonote/BlockTemplate.h#L117
https://github.com/PowPool/xmrpool/blob/main/cnutil/cnutil.go#L11
These were easy to find and will be easy to fix, but how many constants like these are out there and will have to be fixed one by one after they fail?
As for fake/real txs, this varint is not about fake/real. It's about how many txs are in the block template which is an indicator of what the pool is doing. If it says 25, there are 25 transactions of some kind in the block.
3 participants
See docs/BLOCK_HASHING.md, #8827, tevador/RandomX#265, tevador/RandomX#317.
TODO:
Store intermediate hashes in blockchain DBServe intermediate hashes in p2p messages while syncing / notifying of new blocksVerify intermediate PoW before starting full block verificationNotify downstream mining software maintainers (namely xmrig)Update hard fork table for activationDiscussion about overall changes welcome.
RandomX bump requires GCC 14 bump on RISC-V platforms.